-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[canvas] Color fixes + Storybook 5 #34075
Conversation
Pinging @elastic/kibana-canvas |
💔 Build Failed |
Looks good, just a couple of suggestions with regards to the help and placeholder text. Change the helpText prop value to...^ technically, 'HTML color names' is most correct, but for consistency with the placeholder text (below), this could alternatively say 'HTML color codes'Change the placeholder text to then be more generic.. |
💚 Build Succeeded |
Please test this in IE. Last time a storybook PR went in, it broke IE for all of Kibana in master. |
💔 Build Failed |
@snide Which PR broke IE, and how? |
@clintandrewhall This was the fix we added #32159 after storybook was initially added. Just be careful with such a large dependency in Kibana. So if you move it around or update it in anyway, please check it out in various browsers since jenkins doesn't check this kind of stuff. |
@snide good to know, I appreciate the heads up on this. I figured my changes were safe because it was an x-pack dev dependency, but I can see where the problem came from. I tested my component in IE11, but I'll double-check Kibana as well. |
@snide Just checked: it loads and functions... let me know if there's anything specific you'd be concerned about... from the look of the last time this failed, I should see something nasty either starting or loading Kibana... but I'll ask all the same. Jenkins, test this. |
Nope. No worries. It's new too so breaks happen. Just want to make sure you check it out if you ever do a version bump like this one on it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only looking at the color picker changes, they look good and do in fact fix the listed issues.
Not mixing the Storybook stuff with the fixes would have been nice.
💚 Build Succeeded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only checked the styling code. You can make some optimizations if you're trying to stay on point, but the sass changes look OK.
@@ -0,0 +1,3 @@ | |||
.canvasColorPickerPopover__popover { | |||
width: 250px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally we try to avoid pixel values so that we know the designs are based off the EUI math and not brittle. Something like $euiSize * 15
.
...canvas/public/components/color_picker_popover/__examples__/color_picker_popover.examples.tsx
Show resolved
Hide resolved
💔 Build Failed |
💚 Build Succeeded |
@ryankeairns I updated the help text. Let me know if this doesn't match your expectation. |
💚 Build Succeeded |
(Fixes elastic#33734 elastic#33757 elastic#33735) This PR resolves issues with the `mini` color selector, the places the color picker was used, and removes complexity around color selection. It allows for CSS-based color strings while still ruling out duplicates in the color palette, and fixes bugs found recently. This PR also includes + modifies elastic#33896 and updates Storybook to v5. ![Mar-28-2019 09-18-45](https://user-images.githubusercontent.com/297604/55168267-873a1c00-5140-11e9-93b9-f5ca86e70098.gif)
Summary
(Fixes #33734 #33757 #33735)
This PR resolves issues with the
mini
color selector, the places the color picker was used, and removes complexity around color selection. It allows for CSS-based color strings while still ruling out duplicates in the color palette, and fixes bugs found recently.This PR also includes + modifies #33896 and updates Storybook to v5.